Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic Dockerfile #4

Merged
merged 5 commits into from
Jun 21, 2023
Merged

Add basic Dockerfile #4

merged 5 commits into from
Jun 21, 2023

Conversation

rblaine95
Copy link
Collaborator

@rblaine95 rblaine95 commented Jun 20, 2023

  • Add basic Dockerfile
  • Add .tool-versions for asdf/rtx
  • Extend .gitignore
  • Add docker-compose.yaml and Tiltfile

I tried to do a multi-stage Docker build, but the build output depends on the node_modules/ directory.

Partially resolves #1

@rblaine95 rblaine95 self-assigned this Jun 20, 2023
@rblaine95 rblaine95 added the enhancement New feature or request label Jun 20, 2023
@rblaine95 rblaine95 requested a review from wdbasson June 20, 2023 08:47
@rblaine95 rblaine95 force-pushed the dockerfile branch 3 times, most recently from 6494858 to b055f57 Compare June 20, 2023 09:29
Dockerfile Outdated

EXPOSE 3000

ENTRYPOINT [ "tini", "--", "node", "./build/main.js" ]
Copy link
Collaborator Author

@rblaine95 rblaine95 Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that, when using ENTRYPOINT [ "node", "./build/main.js" ], I couldn't send a SIGINT to the node process (CTRL+C) successfully.
Hence wrapping it in tini which handles signals and reaps zombie processes (if there ever are any zombie processes in the future)

@rblaine95 rblaine95 force-pushed the dockerfile branch 3 times, most recently from 0628202 to 3734a43 Compare June 20, 2023 13:03
@rblaine95
Copy link
Collaborator Author

rblaine95 commented Jun 20, 2023

Tilt gives you a web interface with logs and live reloading docker containers on code change (and more)
image

@rblaine95 rblaine95 force-pushed the dockerfile branch 3 times, most recently from 27279a8 to 05c87e5 Compare June 21, 2023 09:29
@rblaine95 rblaine95 marked this pull request as draft June 21, 2023 10:53
@jakubkoci
Copy link
Collaborator

Hi @rblaine95. Great additions 👍 We just need to make the PR to the main branch. I accidentally merged boilerplate to main already. May I ask you for updating or creating a new PR? I had same issue with logger branch. To solved, I created a separate branch logger-new from the current main and then cherry-picked the commit from the logger. I don't know if there is any other or better way.

I'm afraid that if we merge this to boilerplate then we will struggle merging it to the main but maybe it's gonna be fine 🤷‍♂️ I'm sorry for the inconvenience.

@rblaine95 rblaine95 changed the base branch from boilerplate to main June 21, 2023 11:56
@rblaine95
Copy link
Collaborator Author

rblaine95 commented Jun 21, 2023

Sure thing @jakubkoci, I've updated the base of the PR and will do a quick rebase.

* Add basic Dockerfile
* Add `.tool-versions` for `asdf`/`rtx`
* Extend `.gitignore`
* Add vscode extensions recommendations
* Add `.prettierignore` to ignore the `build/` directory
* Build `node_modules` early in Docker image for more efficient caching
* Use `tini` as entrypoint to catch signals
  * I noticed when not using `tini`, I couldn't `SIGINT` the node
    process

asdfg
* Add `NODE_ENV` to Dockerfile
* Remove build from docker-compose as Tilt was already building
* Change docker entrypoint and command
* Re-organize dependencies in `package.json`
  * Many dev dependencies are actually just dependencies
  * Reduce Docker image size by splitting Dev/Prod
@rblaine95 rblaine95 marked this pull request as ready for review June 21, 2023 12:11
Copy link
Collaborator

@jakubkoci jakubkoci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added some notes but LGTM overall. Good job 👍

FROM docker.io/node:18-alpine

USER 0
RUN apk add --update --no-cache tini curl
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a commit with signal handlers to this PR which is a good practice in Node.js apps in general. We shouldn't need tini with them.

I usually tend to lower deps in Docker as much as possible, but if you feel it's still beneficial having tini I'm happy with that :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tini is exceptionally small (±68KB)

The main benefits of tini is that:

  • Reaping of zombie processes (I've seen some docker images that benefit from this)
  • Signal handling
  • Fully transparent

krallin/tini#8 has a good writeup on benefits of using it.

{
"files.trimFinalNewlines": true,
"files.insertFinalNewline": true
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be solved by the Prettier formatter. You can check and update the format with the following commands:

yarn format:check
yarn format:update

Doesn't it work for you? I'll add some pre-push hooks that will check the format and linter rules before each push to the repo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prettier does work for me, but I like to add these settings with vscode as it executes whenever you save the file

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WRT git hooks, I've worked with Husky in the past

}
)

docker_compose('docker-compose.yaml')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't heard of the Tilt yet. Sounds cool 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think docker compose up but on steroids

@rblaine95 rblaine95 merged commit e3fd217 into main Jun 21, 2023
@rblaine95 rblaine95 deleted the dockerfile branch June 21, 2023 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Walking Skeleton
2 participants